Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix flaky HTTP/2 test #41181

Closed
wants to merge 4 commits into from
Closed

Fix flaky HTTP/2 test #41181

wants to merge 4 commits into from

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Apr 13, 2022 •
edited
Loading

I'm taking my own advice from #39588 (comment) and ensuring we lose the AsyncTestSyncContext for the duration of the Kestrel's in-memory HTTP/2 tests. Previously we were just doing this for the duration of InitializeConnectionAsync which fixed some flakiness.

By ensuring the echo app in the AcceptNewStreamsDuringClosingConnection test runs inline with await SendDataAsync(3, _helloBytes, true); and completes, we can ensure the final goaway gets sent without resorting to hacks like calling TriggerTick() in a loop until a timeout.

I'm not sure how applicable this fix will be after @davidfowl's #40925. I think it should still be good because Http2FrameWriter's channel runs inline in tests.

Fixes (but not really fixes because unquarantine rules) #39479 #41172

@ghost ghost added the area-runtime label Apr 13, 2022
@BrennanConroy
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@BrennanConroy
Copy link
Member

This PR seems to have made stuff flaky? Or for some reason flaky tests only show up with this change 😆

The three CI runs have all had different kestrel tests fail.
https://dev.azure.com/dnceng/public/_build/results?buildId=1717072&view=ms.vss-test-web.build-test-results-tab
https://dev.azure.com/dnceng/public/_build/results?buildId=1716614&view=ms.vss-test-web.build-test-results-tab
https://dev.azure.com/dnceng/public/_build/results?buildId=1742997&view=ms.vss-test-web.build-test-results-tab

@halter73 halter73 mentioned this pull request Oct 7, 2022
Merged
@ghost
Copy link

ghost commented Jan 25, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please remove the pending ci rerun label to kick off a new CI run.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 25, 2023
@amcasey
Copy link
Member

amcasey commented Mar 23, 2023

Triage: Closing this for bookkeeping reasons. Feel free to re-open if we find a way to reduce the flakiness.

@amcasey amcasey closed this Mar 23, 2023
@ghost
Copy link

ghost commented Mar 23, 2023

Hi @amcasey. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BrennanConroy BrennanConroy BrennanConroy left review comments

@JamesNK JamesNK JamesNK approved these changes

@Tratcher Tratcher Awaiting requested review from Tratcher

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@halter73 @BrennanConroy @amcasey @JamesNK